-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] Create new Card Details page #45155
[No QA] Create new Card Details page #45155
Conversation
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@allgandalf what is your ETA for the review? |
logging off for dinner, should be back in 1.30 hours |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@mountiny what is the best way for me to test this using the adhoc build? Do I need any betas enabled or anything? |
Ah maybe it would be a bit tricky to test on the adhoc build, you need to have a policy that has the cards feature enabled already Is it ok for you to check the design from the recordings? I think the page looks much similar to the one we have for the exisitng card you own |
Starting Review now 🔁 🟢 |
@VickyStash in the design doc there are 2 more fields
I think we should also just put in a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is almost good, but waiting for some of the comments to be addressed, I will be quick tomorrow to test the changes as soon as @VickyStash updates it
@allgandalf I've skipped it on purpose since there are other issues with implementing this functionality. But I'm okay to add UI elements to the Card Details screen in this ticket. |
@allgandalf I've applied updates and updated recordings, please take a look |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on all platforms, all yours @mountiny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come on melvin ask vit for a review!!
@allgandalf @VickyStash There are conflicts now, sorry for not merging it soon enough |
# Conflicts: # src/languages/en.ts # src/languages/es.ts # src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx # src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts # src/libs/Navigation/linkingConfig/config.ts # src/libs/Navigation/types.ts
@mountiny conflicts resolved, take a look, please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.7-3 🚀
|
🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 9.0.7-4 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀
|
Details
Creates a new Card Details page
Fixed Issues
$ #44325
PROPOSAL: N/A
Tests
Pre-step:
Comment out the feature check in the WorkspaceExpensifyCardDetailsPage file, so you can access the screen.
Steps:
/expensify-card/1
, for example:https://dev.new.expensify.com:8082/settings/workspaces/YOUR_POLICY_ID/expensify-card/1
.Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4